-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tests] Fix recently added mono test #110682
Conversation
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
if (Environment.Is64BitProcess) | ||
{ | ||
Test64Bit (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does moving the code to separate subroutine helps with:
Secondly, inlining of Environment.Is64BitProcess fails due to class initialization not yet being performed.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider code like:
if (true)
print sizeof (T1)
else
print sizeof (T2)
sizeof (T1)
needs to run some class initialization before being computed. Also the size gets included in the emitted code. In this scenario, during method compilation, sizeof (T1)
will be computed and we won't emit any code in the else branch. However if the if
condition is not obviously known at method compile time, then both branches will be compiled and we would end up running initialization for T1
as well as for T2
. This is not about resolving the condition better, but rather preventing the initialization of both types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, thanks. Everything can be solved by another layer of indirection :)
However, thinking about it more, why would if (Environment.Is64BitProcess)
(which is just IntPtr.Size == 8
) be unknown, especially during AOT. Maybe we are not optimizing the compilation and always emitting code for both branches and discarding the else branch later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not the case. We don't inline any methods from classes that have a cctor that hasn't been run yet. In this particular case, the cctor wouldn't have any consequence on the result.
How does CoreCLR handle Environment.Is64BitProcess
for example ? Does it rely on the tiered method to have the optimized code generated while the untiered method doesn't hardcode the value ? @AndyAyersMS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does CoreCLR handle
Environment.Is64BitProcess
for example ? Does it rely on the tiered method to have the optimized code generated while the untiered method doesn't hardcode the value ? @AndyAyersMS
I don't see any special treatment (eg intrisic attribute) in CoreCLR for Is64BitProcess
, so it will be a call in Tier0 and inlined / folded to a constant in Tier1.
There are two issues with the test. Firstly, the
BigArray_x_2
structs were expected to throw type load exception in the test, due to size limit, but their size was decremented by one instead of incremented. Secondly, inlining ofEnvironment.Is64BitProcess
fails due to class initialization not yet being performed. This means that we would end up initializing both 32bit and 64bit types in the test.This test has been failing on mono ever since it was added in #104393. It seems to have gone undercover because it is not being run as apart of normal desktop runtime tests due to incorrect configuration where it is removed in
Loader.csproj
. This hack was apparently done because disabling the test on coreclr hits a separate issue #105964.Test was consistently failing on mobile where we use a different mechanism for running runtime tests.